ECS wrapper for the sound system - #590
Open
IsaacLic wants to merge 9 commits into
Open
Conversation
IsaacLic
marked this pull request as ready for review
March 9, 2021 02:44
NicholasBatesNZ
previously requested changes
Jun 14, 2021
NicholasBatesNZ
left a comment
NicholasBatesNZ
left a comment
Member
There was a problem hiding this comment.
Lots of good work here, thanks. And sorry for the extended delay :(
You may have to explain some things to me if I don't understand it properly :)
Member
# Conflicts: # engine/src/main/java/org/destinationsol/SolApplication.java # engine/src/main/java/org/destinationsol/game/SolGame.java
Addresses the review on MovingBlocks#590. The ECS sound path re-implemented sound selection and playback alongside the existing SolObject path instead of reusing it. It now delegates: - SpecialSounds.hitSound(MaterialType, DmgType) is the single hit-sound selector, replacing both hitSound(boolean, DmgType) and the added getHitSound(MaterialType, DmgType). collisionSound(MaterialType) likewise. Both return Optional rather than null. - SpecialSounds gains playHit and playColl overloads that take an EntityRef. They share selection, thresholds and volume scaling with the SolObject versions; only the position and material sourcing differ, since an entity carries those as components. - SoundPlayingSystem calls those instead of selecting and playing sounds itself, and uses the impulse's contact position for collisions, as SolContactListener already does for SolObjects. - OggSoundManager's two play(...) paths and two skipLooped(...) methods collapse into one generic implementation each, parameterised by the loop-bookkeeping map. The resolved OggSound is returned from the shared method so that a PlayableSound backed by an OggSoundSet is not sampled twice for one playback. - DamageEvent.getDamageType() returns Optional<DmgType>. - DebugHint takes a SolObject, an EntityRef, or neither, through separate constructors instead of arbitrary nulls. SoundPlayingSystem and AsteroidSoundSystem used @in field injection, which the gestalt DI container that instantiates EventReceivers does not populate; they would have thrown NullPointerException at runtime. Both now use javax.inject.Inject, matching the other systems. Also fixes Component.copy -> copyFrom on the new Material component, the getVolume signature change, and a typo in SoundEvent.volumeMultiplier. Co-authored-by: IsaacLic <IsaacLic@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OggSoundManager, OggSoundSet and SpecialSounds existed in both org.destinationsol.game.sound and org.destinationsol.assets.sound. Only the assets.sound copies are wired up: every consumer in engine, desktop and the tests imports org.destinationsol.assets.sound.*, and the game.sound copies were referenced only by each other. They have been dead since the assets.sound versions were introduced. The rest of the game.sound package is untouched. DebugHint and DebugHintDrawer are still live -- assets.sound.OggSoundManager imports DebugHintDrawer -- so the package and its @API export remain. Originally spotted by IsaacLic in MovingBlocks#590. Co-authored-by: IsaacLic <IsaacLic@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 472 files, which is 372 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (472)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NicholasBatesNZ
dismissed
their stale review
August 14, 2026 23:48
I've implemented the changes myself (well Claude but whatever)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is an ECS-based method for playing sounds. It makes use of the existing
OggSoundManager, but adds support for entities. It also adds aMaterialTypeenum, currently only used for determining which sounds to play. In addition, it removes the duplicate, unused classes fromgame/sounds, which were identical to classes with the same names inassets/sounds.There are three types of sound-playing added:
SoundEventcan be sent.This PR also adds in Asteroid's
AsteroidCracksound, which is the sound played when an asteroid gets destroyed.Testing
I wasn't able to test the collision sound locally (my setup has a few issues for me to work out), so that one especially needs testing.
Maintainer update (Aug 2026)
Merged forward onto current
developand the June 2021 review addressed, so this can move again. @IsaacLic's original commits and authorship are preserved in history.Review feedback addressed
The central objection was that the ECS path re-implemented sound selection alongside
SpecialSoundsrather than reusing it. It now delegates:SpecialSoundshas one hit selector,hitSound(MaterialType, DmgType), and one collision selector,collisionSound(MaterialType). Both returnOptionalrather thannull.playHit/playCollgainedEntityRefoverloads sharing selection, the impulse threshold and volume scaling with theSolObjectversions. Only position/material sourcing differs, since an entity carries those as components.SoundPlayingSystemcalls those instead of selecting and playing sounds itself.OggSoundManager's twoplaybodies and twoskipLoopedmethods collapse into one generic private implementation each. All public overloads are unchanged.DamageEvent.getDamageType()returnsOptional<DmgType>.DebugHinttakes aSolObject, anEntityRef, or neither, via separate constructors instead of arbitrarynulls. (This also restoresDebugHint(SolObject, Vector2), which the original branch had widened to a 3-arg constructor.)Other fixes
SoundPlayingSystemandAsteroidSoundSystemused@Infield injection.EventReceiverServiceRegistryhands everyEventReceiverto the gestalt DI container, which only honours@Inject, so both would have thrownNullPointerExceptionat runtime. Converted to matchMoneyDroppingSystem/RubbleBodyCreationSystem.Component.copy→copyFrom, andgetVolume's newSolCamparameter.OggSoundSet.getOggSound()resolves a random sound per call, so the shared play helper takes an already-resolvedOggSound— otherwise the debug overlay could name a different sound than the one played.API changes
assets.sound.SpecialSounds.hitSound(boolean, DmgType)hitSound(MaterialType, DmgType)returningOptional.hitSound(m, d)→hitSound(m ? METAL : ROCK, d).orElse(null). The new overload also accepts an unknown material, which the boolean form could not express.game.sound.SpecialSoundsassets.sound.SpecialSounds— identical fields andplayHit/playCollgame.sound.OggSoundSetassets.sound.OggSoundSet— identical membersgame.sound.OggSoundManagerassets.sound.OggSoundManager— obtained via DI rather thannew;update(SolGame)→update(SolGame, float)Every other public/protected symbol on
developis unchanged; verified byjavap-diffing each touched class againstdevelop. Everything else here is additive. Thegame.sounddeletions are the last commit, kept standalone so they can be dropped or taken separately.Testing
./gradlew clean :engine:compileJava :desktop:compileJava :engine:test— 105 tests, 0 failures, 1 skipped.Verified in-game with
spawnECSAsteroid=trueandsoundInfo=true:asteroidCrack.Known limitations (pre-existing, not introduced here)
Projectileonly sends aDamageEventwhen the obstacle is aSolObjectEntityWrapper.PointProjectileBodywrapsEntityRefuser data explicitly (with aTODOacknowledging it as a patch);BallProjectileBodyhas no equivalent. So weapons whose projectile config setsphysSize > 0pass through entities without dealing damage — and therefore play no hit or death sound. This is a gameplay gap independent of sound and is unchanged by this PR.RubbleCreationSystemcreates rubble entities without aMaterialcomponent, so the new sound handlers never match them. AddingMaterial(ROCK)there would make this audible in ordinary play, but that touches recently-merged work and is left as a follow-up.SolApplicationis still the only entity in the game carrying aMaterialcomponent, so this feature is not yet reachable in normal gameplay.